-
Notifications
You must be signed in to change notification settings - Fork 8
Witness Validation Footgun #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for not being afraid of the blank page!
src/ledger/ledger-rules/README.md
Outdated
| # Ledger Rules | ||
| This section details the ledger rules used in Cardano and, especially, some of the potential "foot guns" one could run into when implementing these rules. Those "foot guns" could be discrepencies between the specification and the ledger, some undefined behavior, or a bug that was fixed at some point, but is still relevant for syncing historical data. | ||
|
|
||
| It is an ongoing work in progress, being updated periodically in paralel with the work happening to build Amaru. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is a very useful introduction yet. It seems you intend to describe only the discrepancies between specification and the haskell implementation? Can you please add links to those here?
Have you considered replacing / extending ledger/README.md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I provided some more context. I did not consider replacing extending the ledger/README.md just yet, as I am trying to keep this piece of work isolated from the rest of it at least for now. Once it's mostly completed, or in a better state, it probably makes sense to find a way to merge the two together, but for now I wanted to avoid the additional complexity of making it fit.
| @@ -0,0 +1,81 @@ | |||
| # Witness Validation | |||
|
|
|||
| TODO: provide a description of the ledger rules relevant to this domain specific concept. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid you need to resolve this as I don't get the "foot gun" due to the lack of context (and I consider myself a good example of a potential reader for this being familiar with the EUTxO ledger model, but not have the details ready).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps "foot gun" is the wrong term for this particular example. It's something that's easy to get wrong, but technically the logic matches the specification. One would probably assume that bootstrap_witnesses contains all the bootstrap witnesses (as I did), but that would be a wrong assumption.
With very specific payloads, you can provide a vkey witness for a bootstrap address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it then more a "lesson learned", a "gap", a "I wish I would have known" or a "hard won wisdom"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the term to "Land Mine" as it's not as well defined a term as "Footgun", which doesn't feel quite right.
In this case a landmine is just anything that can/has caused some additional confusion because the correct implementation is not necessary the most obvious one. That could be a result of a discrepancy between the haskell node and the specification, a bug, some unintended behavior (as I think this one is since the spec doesn't have the notion of a bootstrap_witness), or just something that tripped us (or someone else up) and could be helpful to have some clarification on documented somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry but land mine has a very lethal touch to me and sounds very negative.
From your explanations I would have called it a "pitfall". Something you happen to document once you "fell into the pit" when implementing a ledger? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a repeated spelling error while reading through this
| ### Key Definitions | ||
| **`Witness`**: A witness is a piece of data that allows you to verify the authenticity of the transaction. There are many different witness types, depending on what the transaction requires, such as vkey witnesses, bootstrap witnesses, and script witnesses. | ||
|
|
||
| **`Vkey Witness`**: A vkey witness is a specific witness type. It is used to verify that the transaction has the authority to consume a UTxO(s) locked at the assosciated `pkh` address. It includes the signature and the vkey (instead of the key hash, which is encoded in the address). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"associated" (spelling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be caught by a new (rudimental) github action now: https://github.com/cardano-scaling/cardano-blueprint/actions/runs/16768824718/job/47479321101?pr=49
|
|
||
| **`Vkey Witness`**: A vkey witness is a specific witness type. It is used to verify that the transaction has the authority to consume a UTxO(s) locked at the assosciated `pkh` address. It includes the signature and the vkey (instead of the key hash, which is encoded in the address). | ||
|
|
||
| **`Bootstrap Witness`**: A bootstrap witness is a specific witness type. It is used to verify that the transaction has the authority to consume a UTxO(s) locked at the associated Byron-era address. It includes the assosciated public key, the signature, the 32 byte chain code, and a set of attributes encoded in the address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"associated" (spelling)
| Nothing -> ans | ||
| ``` | ||
|
|
||
| The Haskell node is inserting hashes from both `AddrBootstrap` and `Addr` into the same set, since they are both just hash digests of the same size. The hash digest inserted in the case of an `AddrBootstrap` is the bytes that are found at the first field of the `BYRON_ADDRESS_PAYLOAD`. In theory, this is the digest of the `blake2b_224(sha3_256(BYRON_ADDRESS_ROOT))` and that should never be the same digest as a `Addr` vkey hash digest, so there would be no complexity introduced here. However one could, and clearly did, manually construct a Byron-era address where the bytes that are suppose to be the digest of the `BYRON_ADDRESS_ROOT` are instead the digest of a vkey. In that case, a vkey witness present with the assosciated public key would satifsy the required witness, even though that requirement came from an Byron-era bootstrap address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"associated" (spelling)
| ## "Land Mine" #1: Bootstrap Witnesses | ||
|
|
||
| ### Context | ||
| After Shelley introduced the [`bootstrap_witness`](https://github.com/IntersectMBO/cardano-ledger/blob/d71d7923a79cfcde8cac0b5db399a5427524d06a/eras/shelley/impl/cddl-files/shelley.cddl#L296-L301) field to the `transaction_witness_set`. One may reasonably assume that if a Byron-era address is present, the assosciated witness would be found in the `bootstrap_witness` field. However, it is possible to construct a valid transaction such that it includes a Byron-era address, but the `bootstrap_witness` field is an empty list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"associated" (spelling)
|
@yHSJ I still want to see this contribution find its way into the blueprint. The ledger chapter has moved ahead since and has gotten an introduction page and @nc6 is actively working on adding more and more things following a plan he has presented here. Could you provide your feedback on that plan and maybe some guidance how these footguns (or pitfalls?) would fit into the existing and envisioned chapter? Also, do you want to become a code owner for |
a054816 to
bbdaac5
Compare
|
@ch1bo Yes, I'd love to see this information included as well. Sorry for my absence, I've been busy and then traveling for work (Argentina and now Vegas). I'll try to find some time this week to take a look at stuff, but most likely I'll focus on this early next week. Yes, I'd love to become a code owner! The only question is what type of a commitment I would be making -- I don't like to commit to something and then give it half effort. |
This PR introduces some "scaffolding" for how we could document ledger rules. I've organized them by "domain concepts" instead of any particular organization of the ledger rules, and we could provide some prose about what the ledger rules are requiring, along with linking to the specification and implementation(s).
I haven't done this for witness validation, but I did describe a "foot gun" that @KtorZ discovered while fixing a bug in Amaru yesterday. I think this is a reasonable framework for documenting this kind of work, but I am very open to suggestions and feedback.